[nodejs] create new weblog for express 5 #3572

IlyasShabi · 2024-11-27T19:51:32Z

Motivation

Express has released a new version, v5, which includes some breaking changes compared to v4. We have a PR ready for review and merging on dd-trace-js. Additionally, we need to create a weblog for Express 5 to identify potential breaking changes and ensure our instrumentation remains compatible.

Changes

Change express4 folder name to express
Change all express dockerfile configuration
Exclude graphql and mongo sanitize from express 5

Workflow

⚠️ Create your PR as draft ⚠️
Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
Mark it as ready for review
- Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
- Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

If PR title starts with [<language>], double-check that only <language> is impacted by the change
No system-tests internal is modified. Otherwise, I have the approval from R&P team
CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
A docker base image is modified?
- the relevant build-XXX-image label is present
A scenario is added (or removed)?
- Get a review from R&P team

IlyasShabi · 2024-11-28T14:36:09Z

manifests/nodejs.yml

@@ -558,84 +563,106 @@ tests/:
        Test_Kafka:
          '*': irrelevant
          express4: v0.1 # real version not known
+          express5: v0.1 # real version not known


Question: Versioning is more related to features rather than instrumentation or weblogs. Do you think I should use the same versions as express 4?

As express5 is not yet supported, I think that you can't set any version <5.29.0 or <4.53.0 (next release) for any test.

IlyasShabi · 2024-11-29T07:44:59Z

utils/scripts/load-binary.sh

@@ -215,7 +215,7 @@ elif [ "$TARGET" = "agent" ]; then
 elif [ "$TARGET" = "nodejs" ]; then
    assert_version_is_dev
    # NPM builds the package, so we put a trigger file that tells install script to get package from github#master
-    echo "DataDog/dd-trace-js#master" > nodejs-load-from-npm
+    echo "DataDog/dd-trace-js#express5" > nodejs-load-from-npm


Should be removed after review

utils/build/docker/nodejs/express4-typescript.Dockerfile

uurien · 2024-11-29T08:02:09Z

utils/build/docker/nodejs/express4.Dockerfile


 WORKDIR /usr/app

 ENV NODE_ENV=production

 RUN npm install
+RUN npm install "express@^4.17.2" "apollo-server-express@^3.13.0" "express-mongo-sanitize@^2.2.0"


as package-lock.json will not work for these dependencies, should we fix the versions?

Suggested change

RUN npm install "express@^4.17.2" "apollo-server-express@^3.13.0" "express-mongo-sanitize@^2.2.0"

RUN npm install "[email protected]" "[email protected]" "[email protected]"

uurien · 2024-11-29T08:02:31Z

utils/build/docker/nodejs/express5.Dockerfile

+ENV NODE_ENV=production
+
+RUN npm install
+RUN npm install "express@^5.0.1"


as package-lock.json will not work for these dependencies, should we fix the versions?

RUN npm install "[email protected]"

CarlesDD

missing_feature decorator for this test should be addressed to check the new weblog variant for express5.

utils/build/docker/nodejs/express/package.json

cbeauchesne

I'm not a node expert, but would it be more reliable to have a package/lock per weblog, rather a common one + a dedicated npm install ?

cbeauchesne · 2024-11-29T13:26:36Z

tests/appsec/iast/source/test_parameter_name.py

@@ -37,7 +40,10 @@ def test_source_get_reported(self):
        """ for use case where only one is reported, we want to keep a test on the one reported """
        self.validate_request_reported(self.requests["GET"])

-    @missing_feature(weblog_variant="express4", reason="Tainted as request body")
+    @missing_feature(


This change is surprising, it goes from weblog_variant == "express4" to weblog_variant not in ["express4" ...). Is it intended ?

Thanks I will fix this, the feature is currently marked as missing in nodejs.yml
The failing test in nodejs prod is expected as my PR hasn't been merged yet.

ok, passing the PR as draft to unstress my noti stack. You can set it back to normal once it's ready to be merged.

cbeauchesne

Some CI failures to fix, probably related to my comment.

IlyasShabi added 3 commits November 27, 2024 20:47

[nodejs] create new weblog for express 5

4c55e3a

fix lint issues

064ed66

target express 5 version for dev tests

4484f99

IlyasShabi commented Nov 28, 2024

View reviewed changes

IlyasShabi added 5 commits November 28, 2024 18:00

fix express weblog order

f7d90d3

remove express 5 folder and use same as express 4

76a5f69

rename express folder

8f5659d

fix linter

013e560

fix unnamed wildcard for express 4

0f840fb

IlyasShabi marked this pull request as ready for review November 29, 2024 07:44

IlyasShabi requested review from a team as code owners November 29, 2024 07:44

IlyasShabi commented Nov 29, 2024

View reviewed changes

CarlesDD reviewed Nov 29, 2024

View reviewed changes

utils/build/docker/nodejs/express4-typescript.Dockerfile Outdated Show resolved Hide resolved

uurien reviewed Nov 29, 2024

View reviewed changes

CarlesDD reviewed Nov 29, 2024

View reviewed changes

utils/build/docker/nodejs/express/package.json Outdated Show resolved Hide resolved

cbeauchesne reviewed Nov 29, 2024

View reviewed changes

fix packages versions

3e026f8

IlyasShabi force-pushed the express5-weblog branch from 097c3e1 to 3e026f8 Compare November 29, 2024 10:47

cbeauchesne reviewed Nov 29, 2024

View reviewed changes

cbeauchesne requested changes Nov 29, 2024

View reviewed changes

cbeauchesne marked this pull request as draft November 29, 2024 13:54

fix nodejs.yml express5 versions

43f01eb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nodejs] create new weblog for express 5 #3572

[nodejs] create new weblog for express 5 #3572

IlyasShabi commented Nov 27, 2024 •

edited

Loading

IlyasShabi Nov 28, 2024 •

edited

Loading

uurien Nov 29, 2024

IlyasShabi Nov 29, 2024

uurien Nov 29, 2024

IlyasShabi Nov 29, 2024

uurien Nov 29, 2024

IlyasShabi Nov 29, 2024

CarlesDD left a comment •

edited

Loading

cbeauchesne left a comment

cbeauchesne Nov 29, 2024

IlyasShabi Nov 29, 2024

cbeauchesne Nov 29, 2024

cbeauchesne left a comment

	RUN npm install "express@^4.17.2" "apollo-server-express@^3.13.0" "express-mongo-sanitize@^2.2.0"
	RUN npm install "[email protected]" "[email protected]" "[email protected]"

[nodejs] create new weblog for express 5 #3572

Are you sure you want to change the base?

[nodejs] create new weblog for express 5 #3572

Conversation

IlyasShabi commented Nov 27, 2024 • edited Loading

Motivation

Changes

Workflow

Reviewer checklist

IlyasShabi Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlesDD left a comment • edited Loading

Choose a reason for hiding this comment

cbeauchesne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeauchesne left a comment

Choose a reason for hiding this comment

IlyasShabi commented Nov 27, 2024 •

edited

Loading

IlyasShabi Nov 28, 2024 •

edited

Loading

CarlesDD left a comment •

edited

Loading